Match plugin text/axis/icon colours to the napari theme. #138

samcunliffe · 2023-05-31T14:00:49Z

Change the text, axes, and icon colour to contrast with the background. I hear white text on an off-white background is not ideal.

Solves

Hard-coded icons and axes looks bad with non-default napari themes #86

Testing:

The theme changing via the viewer.events.theme callback is currently only tested manually. It works better than the current main but is still not perfect. (I'm going to try and make a gif to show what I mean.)
The icon path stuff is unit tested ✅

Note for reviewer(s):

This somewhat supersedes my plan (and what I wrote in the PR title) for Generalise stylesheet parsing to extract colours for light/dark theme callback. #105 but doesn't actually supersede the code changes.
- ...so I'm going to keep that open for a bit longer (sorry for polluting the PR list). Will close when I decide what to do with it.
Are you OK with two face colours? I use both theme.text and theme.foreground.
The icons are black and not the subtle brownish grey used by napari's light theme.
The napari.Viewer gets moved up the family tree and becomes a member of the BaseNapariMPLWidget.

samcunliffe · 2023-05-31T16:00:10Z

src/napari_matplotlib/base.py

+        """
+        self._replace_toolbar_icons()
+        if self.figure.gca():
+            self.apply_napari_colorscheme(self.figure.gca())


Not sure if this is the correct line, but I still don't quite have a perfect UX when switching the theme whilst the widgets are open...

Screen.Recording.2023-05-31.at.16.51.00.mov

Re-opening or doing something to the plot seems to fix.

A bit of a dive into the napari source, and you might want to use something like this line to trigger something when the theme is changed?

https://github.com/napari/napari/blob/46dcc9c0976c35ec9dc64be60235f4addcadaa5c/napari/_qt/qt_main_window.py#L578

😞

napari.settings.get_settings().appearance.events.theme.connect( self._on_theme_change )

gives the same behaviour. I wonder if it might be a different way to do the same thing as

self.viewer.events.theme.connect(self._on_theme_change)

The theme-change event is undocumented* (scroll a bit from the start of § Viewer events) so I'd be up for asking and/or fixing the docs for napari.

*) It's also duplicated: that page is made via a script in napari/docs.

ruaridhg

LGTM. Could switching the theme whilst widgets are open be something to do with the napari bug I raised

src/napari_matplotlib/base.py

samcunliffe · 2023-05-31T17:04:32Z

Could switching the theme whilst widgets are open be something to do with the napari bug I napari/napari#5847

I think not. Although something missing in the callbacks for a widget being loaded .... maybe.

samcunliffe · 2023-05-31T17:33:38Z

Also needs a note in the changelog.

@samcunliffe : add a note in the changelog.

Since we're now setting the axes to off-white.

…t-themes

src/napari_matplotlib/tests/baseline/test_example_q_widget.png

samcunliffe · 2023-06-01T08:27:18Z

Err if this is ok to merge, I think it needs a reapproval.

I pushed a release note and updated the pytest-mpl plots. Sorry for the noise.

dstansby · 2023-06-01T08:38:41Z

Happy to merge this (once I've checked the code), but things that need issues opening to be fixed after this:

If I switch to the light theme, the plot looks fine but the whole widget still has a dark background.
We couldn't get the theme changing event to work. Probably upstream issue, but also we should open an issue here so we don't forget.

dstansby

Some minor comments, but overall looking 👍

src/napari_matplotlib/base.py

samcunliffe · 2023-06-01T08:43:01Z

but things that need issues opening to be fixed after this

Yeah. There are a few minor peripheral things that I noticed too...

Axis titles are always black.
Icons are black (and should properly be greyish brown to match the theme).

Are you ok with 3 individual minor issues?

dstansby · 2023-06-01T08:46:36Z

Yep, three issues sounds good

Solves matplotlib#86.

499c0a7

samcunliffe added the Bug Something isn't working label May 31, 2023

samcunliffe requested review from dstansby and ruaridhg May 31, 2023 14:00

Pre-commit and some docstring formatting.

4f8aa6d

samcunliffe linked an issue May 31, 2023 that may be closed by this pull request

Hard-coded icons and axes looks bad with non-default napari themes #86

Closed

samcunliffe commented May 31, 2023

View reviewed changes

ruaridhg approved these changes May 31, 2023

View reviewed changes

samcunliffe commented May 31, 2023

View reviewed changes

src/napari_matplotlib/base.py Show resolved Hide resolved

samcunliffe added 3 commits May 31, 2023 18:59

Have to regenerate the baseline figures.

4e198e7

Since we're now setting the axes to off-white.

Merge remote-tracking branch 'upstream/main' into 86-handling-of-ligh…

b6d2894

…t-themes

One more reference to update.

d5c05c6

samcunliffe force-pushed the 86-handling-of-light-themes branch from 276d6ac to d5c05c6 Compare May 31, 2023 18:01

samcunliffe commented May 31, 2023

View reviewed changes

src/napari_matplotlib/tests/baseline/test_example_q_widget.png Outdated Show resolved Hide resolved

samcunliffe added 2 commits May 31, 2023 19:05

Note to the changelog.

f5b4c2e

No intersphinx for you. Fine.

91d6937

samcunliffe enabled auto-merge June 1, 2023 08:26

dstansby reviewed Jun 1, 2023

View reviewed changes

src/napari_matplotlib/base.py Show resolved Hide resolved

src/napari_matplotlib/base.py Outdated Show resolved Hide resolved

samcunliffe mentioned this pull request Jun 1, 2023

Theme change doesn't work perfectly whilst widget is open. #140

Closed

Better variable names. Add a TODO note about matplotlib#140.

010a02e

dstansby approved these changes Jun 1, 2023

View reviewed changes

samcunliffe added this pull request to the merge queue Jun 1, 2023

Merged via the queue into matplotlib:main with commit 7bf5db1 Jun 1, 2023

This was referenced Jun 1, 2023

Axis titles are always black. #142

Closed

Tests for background lightness checking. #143

Merged

samcunliffe deleted the 86-handling-of-light-themes branch June 1, 2023 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Match plugin text/axis/icon colours to the napari theme. #138

Match plugin text/axis/icon colours to the napari theme. #138

samcunliffe commented May 31, 2023 •

edited

Loading

samcunliffe May 31, 2023 •

edited

Loading

dstansby May 31, 2023

samcunliffe May 31, 2023 •

edited

Loading

ruaridhg left a comment

samcunliffe commented May 31, 2023

samcunliffe commented May 31, 2023 •

edited

Loading

samcunliffe commented Jun 1, 2023

dstansby commented Jun 1, 2023 •

edited

Loading

dstansby left a comment

samcunliffe commented Jun 1, 2023

dstansby commented Jun 1, 2023

Match plugin text/axis/icon colours to the napari theme. #138

Match plugin text/axis/icon colours to the napari theme. #138

Conversation

samcunliffe commented May 31, 2023 • edited Loading

samcunliffe May 31, 2023 • edited Loading

Choose a reason for hiding this comment

dstansby May 31, 2023

Choose a reason for hiding this comment

samcunliffe May 31, 2023 • edited Loading

Choose a reason for hiding this comment

ruaridhg left a comment

Choose a reason for hiding this comment

samcunliffe commented May 31, 2023

samcunliffe commented May 31, 2023 • edited Loading

samcunliffe commented Jun 1, 2023

dstansby commented Jun 1, 2023 • edited Loading

dstansby left a comment

Choose a reason for hiding this comment

samcunliffe commented Jun 1, 2023

dstansby commented Jun 1, 2023

samcunliffe commented May 31, 2023 •

edited

Loading

samcunliffe May 31, 2023 •

edited

Loading

samcunliffe May 31, 2023 •

edited

Loading

samcunliffe commented May 31, 2023 •

edited

Loading

dstansby commented Jun 1, 2023 •

edited

Loading